Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove un-necessary restriction to pointer contracts on EVM -> CW calls for CW20/CW721 pointers #1846

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove un-necessary restriction to pointer contracts on EVM -> CW calls for CW20/CW721 pointers #1846

wants to merge 2 commits into from

Conversation

lordshisho
Copy link

@lordshisho lordshisho commented Sep 7, 2024 •
edited
Loading

Describe your changes and provide context

If a pointer is deployed, any calls to an underlying CW20/CW721 revert if they are made through any contract that is not the pointer.

if caller.Cmp(callingContract) != 0 {
    erc20pointer, _, erc20exists := p.evmKeeper.GetERC20CW20Pointer(ctx, contractAddrStr)
    erc721pointer, _, erc721exists := p.evmKeeper.GetERC721CW721Pointer(ctx, contractAddrStr)
    if (!erc20exists || erc20pointer.Cmp(callingContract) != 0) && (!erc721exists || erc721pointer.Cmp(callingContract) != 0) {
        return nil, 0, fmt.Errorf("%s is not a pointer of %s", callingContract.Hex(), contractAddrStr)
    }
}

If there is no pointer deployed, which is the case of any CW20/CW721 before a pointer is deployed and registered for it, these calls do not revert.

Users should be permitted to make these calls from any contract or custom pointer solution.

@lordshisho lordshisho changed the title Remove un-necessary restriction to pointer contracts on EVM -> CW calls for ERC20/ERC721 pointers Remove un-necessary restriction to pointer contracts on EVM -> CW calls for CW20/CW721 pointers Sep 7, 2024
@dvli2007
Copy link
Contributor

There's good reason for allowing only a single canonical pointer address to exist. The pointer contract is used for stuff such as events emissions mappings. Given that events mappings are working in 5.8.0, I do not think we should open up this can of worms right now for supporting simultaneous pointer contracts.

I vote that we close this PR.

cc: @cordt-sei @lordshisho

@lordshisho
Copy link
Author

There's good reason for allowing only a single canonical pointer address to exist. The pointer contract is used for stuff such as events emissions mappings. Given that events mappings are working in 5.8.0, I do not think we should open up this can of worms right now for supporting simultaneous pointer contracts.

I vote that we close this PR.

cc: @cordt-sei @lordshisho

The canonical pointer would still exist, and it would still transfer the underlying cw20/721 generating the mapped event. If a user is not looking for the non-canonical event, they won't have to worry about it and the canonical event is still relevant.

Pointer creation only checks if the underlying contract basically loosely conforms to the spec. so if you have a custom implementation (non-standard ERC20) you are now locked out of non-standard functions due to this restriction.

@cordt-sei
Copy link
Contributor

cordt-sei commented Sep 13, 2024 •
edited
Loading

@dvli2007 Not all developers are wanting to use the pointer contracts for everything, nor the precompiles in general. Seems like a reasonable ask, as long as it is made clear that self-deployed custom pointers will not carry the events mapping features of integrated pointers.
In reality, all events data exists by default and can be fetched by anyone if there is a need for it. It's just a matter of being willing to find and use the existing data regardless of which flavor of endpoint it's served over.

As long as there is no obvious security risk or additional maintenance overhead, it's not really productive to add arbitrary limitations or attempt to encourage or force usage of specific tooling/methods.

@lordshisho Is this a state-breaking change?
image

@lordshisho
Copy link
Author

@dvli2007 Not all developers are wanting to use the pointer contracts for everything, nor the precompiles in general. Seems like a reasonable ask, as long as it is made clear that self-deployed custom pointers will not carry the events mapping features of integrated pointers.

In reality, all events data exists by default and can be fetched by anyone if there is a need for it. It's just a matter of being willing to find and use the existing data regardless of which flavor of endpoint it's served over.

As long as there is no obvious security risk or additional maintenance overhead, it's not really productive to add arbitrary limitations or attempt to encourage or force usage of specific tooling/methods.

@lordshisho Is this a state-breaking change?

image

I wouldn't think so.

And the event would still fire on the canonical pointer since these custom pointers still interact with the underlying cw contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lordshisho @dvli2007 @cordt-sei